Can probably be simplified to something like: context is an optional field to pass extra instructions to the alert processor on how to route the alert.

hah we need to start splitting these out

^^ @jacknagz I told him to do this for now. I also told @armtash to do it for his JIRA output stuff. I will break this out after these merge

nit: can this be a single line?

you can just declare Keyword Args instead of Args

what if context isn't in the alert? does it always need to be defined for this output?

or rather, what if it's an empty map?

to jack's point, the default for this will be an empty dict, given this change: https://github.com/airbnb/streamalert/pull/467/files#diff-30cc622fccfff0217d84af2ff5bcd7ffR74

Even it is defaulted to an empty dict, I will add a test for that because the output should just take the default escalation policy if no context is passed.

nit: use consistent line breaks

these two lines seem to be repeated in this function, can this be extracted into a helper?

Add a test case for an empty context arg

Please add an expected schema for the context either here or in the class docstring

^^ @jacknagz I told him to do this for now. I also told @armtash to do it for his JIRA output stuff. I will break this out after these merge

to jack's point, the default for this will be an empty dict, given this change: https://github.com/airbnb/streamalert/pull/467/files#diff-30cc622fccfff0217d84af2ff5bcd7ffR74

Remove this else and add the default value to the get(..) above:

user_to_assign = rule_context.get('assigned_user', False)

are the type and details keys within this required? It looks like these values never get set, and this is just passed as the `body in the incident payload

If these are required, can you add a comment saying so and why?

They are required, I just forgot about them :)

I'd be a fan of just moving this dict and the above dict right into the structure below for the incident.

I was digging into the API docs and it turns out that this is just optional, so maybe something else to provide via context.

move this above the else so it's set by default and then just set it, like so:

policy_to_assign = creds['escalation_policy']
if not user_to_assign and rule_context.get('assigned_policy'):
    policy_to_assign = rule_context.get('assigned_policy')

one less line :)

this dispatch function is huge - can you move some of it to functions to make it more readable and make testing easier

this can be simplified to context = context or {}

suggestion: just set self.headers right here (or self._headers once the instance properties are made protected)

Maybe make these instance properties protected? ie - self._headers, etc.. since they should never be accessed from outside an instance of this class.

Suggestion: returning early if there is ever the opportunity to do so, instead of having to do compound if checks throughout a function helps with decrease confusion and increase readability. For instance, this could be make more readable via:

response = resp.json()
if not response:
    return False           # Maybe even add a LOGGER.error(...) here (?)

# If there are results, get the first occurence from the list
target = response.get(target_key, False)
if not target:
    return False           # Maybe add a LOGGER.error(...) here too (?)

return target[0]['id']

Theoretically the last if/return can be consolidated into just:
return response['target_key'][0]['id'] if 'target_key' in response else False

Just food for thought :)

I like replacing the last if/else with that super pythonic statement

I think this args list needs updated to only include service

@javuto still seeing api_url in this list but this isn't an arg in the function declaration 🤔

Is there a chance that this service won't exist within the rule_context dict? It looks like this is performed without a get so is it safe to assume there will always be a value in it for this service? (I don't think this is the case so just making sure I'm not missing something?)

I presumed that the service will be there, but it is a good idea to make sure and add some checks. Nice catch!

This is the context that is sent from the rule processor, correct? Cause I'm thinking that will only rarely get set in some rules, no?

Yeah that would be the approach. The context element will be there, even if it is just an empty dict but the __service__ could be missing. Better do the check.

Could we break out the logic below into a function that returns a tuple of the user_to_assign and the assigned_value? just trying to understand this flow better

with a lookup like this, you can use a default value with the get like context.get('assigned_policy', self._escalation_policy) instead of doing the ... or ...